Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI: Various improvements concerning user details #6173

Merged
merged 5 commits into from
Nov 12, 2023

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 10, 2023

This PR consists of a number of commits that are purposefully kept separate. For review, it would make most sense to look at those commits one by one. If this is too complicated, I am willing to split it up in separate PRs but since they all touch the same files I kept it together for now.

The last commit is what is I really need for #6148
We recently fixed a bug in the verdi quicksetup/setup command where it was possible to not provide anything for the users first name, last name and institution. They are now properly required, but that poses a problem for the UX for the new SqliteDosStorage. The idea is that creating such a profile is dead easy, but due to this change, the user is now forced to specify these options, where often we don't really care.

The last commit adds a default for these options, solving the problem. The other commits are various improvements to the verdi user commands and an actual bug fix when changing default users in a persistent Python interpreter.

@mbercx do you want to take this one perhaps since you have been an advocate of not requiring the user details on setup?

This is a useful shortcut to determine whether a `User` instance is the
current default user. The previous way of determing this was to retrieve
the default user from the collection `User.collection.get_default()` and
manually compare it with the `User` instance.
Uses the `tabulate` package to create a nicely formatted table as is
used in many other `verdi` commands already. The results are ordered by
the users emails.
Each profile can define which user in its storage backend should be
considered the default. This is necessary because each ORM entity, when
created, needs to specify a `User` object and we don't want the user to
always have to explicitly define this manuallly.

The default user for a profile is stored in the configuration file by
the email of the `User` object. However, in a loaded storage backend,
this default user is also cached as the `User` object. This means that
when the default user is changed, it should be changed both in the
configuration file, but if a storage backend is loaded, the cache should
also be invalidated, such that the next time the default user is
requested, the new one is properly loaded from the database.

Since this change affects both the configuration as well as the
currently loaded storage, the `set_default_user_email` is added to the
`Manager` class, since that controls both. It calls through to the same
method on the `Config` class, which is responsible for updating the
`Config` instance in memory and writing the changes to disk. Then the
manager resets the default user on the storage backend, if any is
loaded.

The `verdi user set-default` command is updated to use the new method. A
test is added for the command, which didn't exist yet. The command is
updated to use `Manager.set_default_user_email` even though it could use
`Config.set_default_user_email` since the Python interpreter will shut
down immediately after anyway. However, the test would fail if the
latter would be used, since the loaded storage backend would not have
been updated, which is used by `User.collection.get_default()`. This
demonstrates why in active Python interpreters only the method on the
manager should be used. A warning is added to the docstring on the
configuration class.
@mbercx
Copy link
Member

mbercx commented Nov 10, 2023

@mbercx do you want to take this one perhaps since you have been an advocate of not requiring the user details on setup?

For sure, thanks @sphuber, always happy to help lower the barrier for users to get started with AiiDA. ^^ The message of that final commit is music to my ears.

Quick first question (which I probably asked before): why can the email not be changed later on?

EDIT: Ah, I see, is the email sort of used as an identifier, since:

@property
def uuid(self) -> None:
"""
For now users do not have UUIDs so always return None
"""
return None

@sphuber
Copy link
Contributor Author

sphuber commented Nov 10, 2023

Quick first question (which I probably asked before): why can the email not be changed later on?
EDIT: Ah, I see, is the email sort of used as an identifier, since:

Exactly, the DbUser model doesn't have a UUID but the email column is made unique and is kind of used as a substitute. That being said, it is currently completely possible to change the email of a User in the API 😅 . You can do the following just fine:

In [1]: u = User.collection.get(id=1)
Out[1]: <aiida.orm.users.User at 0x7fd6ad348a90>

In [2]: u.email = 'test@localhost'

So we might want to consider making the email immutable. But maybe it would be even better to keep it mutable and simply add a proper UUID to the DbUser table. I am not sure that we should remove the uniqueness constraint of the email column though, since the default_user uses the email currently. That will break if it is no longer unique.

There are also potential problems lying in wait with archives: what if you want to import an archive that happens to have a user with the same email? Currently you can't. Seems a bit annoying though and I am suprised it hasn't actually cropped up before. Or maybe the import mechanism has a way of dealing with it and automatically changes the email for the imported user. And what happens if you import a user with the same email that actually represents the same user, but the name and institution have changed. Which version do you keep?

Anyway, it is clear that this requires some more thought, but that is for a later time.

@mbercx
Copy link
Member

mbercx commented Nov 10, 2023

Thanks for the explanation @sphuber!

Anyway, it is clear that this requires some more thought, but that is for a later time.

Hehe, yeah, even I wouldn't dare to introduce such scope creep in a PR. ;) So I won't derail the discussion here, but do we have an open issue about this topic? Or a discussion somewhere? I couldn't immediately find one.

@sphuber
Copy link
Contributor Author

sphuber commented Nov 10, 2023

Hehe, yeah, even I wouldn't dare to introduce such scope creep in a PR. ;) So I won't derail the discussion here, but do we have an open issue about this topic? Or a discussion somewhere? I couldn't immediately find one.

No, not that I recall. I will open one now

highlight = lambda x: x.email == default_user.email if default_user else None
echo.echo_formatted_list(User.collection.all(), attributes, sort=sort, highlight=highlight)
if User.collection.get_default() is None:
echo.echo_warning('No default user has been configured')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if this ever can happen. One needs to set up a user to set up a profile, and that would immediately set this user as the default. If you haven't set up a profile, you get:

❯ verdi user list
Critical: no default profile defined: None
{'CONFIG_VERSION': {'CURRENT': 9, 'OLDEST_COMPATIBLE': 9}, 'profiles': {}, 'options': {'autofill.user.email': 'idontwantto@tell.you', 'autofill.user.first_name': 'John', 'autofill.user.last_name': 'Doe', 'autofill.user.institution': 'Unknown'}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is possible. If you delete the user, or you simply change the user's email, or even if the config is corrupted and the default_user_email key is removed. Lot's of possibilities :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I guess you're right

❯ verdi user list
    Email      First name    Last name    Institution
--  ---------  ------------  -----------  -------------
    mwhahahha  John          Doe          Unknown

Warning: No default user has been configured

But these paths are probably not really desirable, since once you don't have a default user, you can't even store a simple Int node.

Side tangent: I checked out the docstring of the store method, and it thoroughly confused me.

Utter confusion
In [4]: Int(1).store?
Docstring:
Lightweight persistence for python variables.

Example::

  In [1]: l = ['hello',10,'world']
  In [2]: %store l
  Stored 'l' (list)
  In [3]: exit

  (IPython session is closed and started again...)

  ville@badger:~$ ipython
  In [1]: l
  NameError: name 'l' is not defined
  In [2]: %store -r
  In [3]: l
  Out[3]: ['hello', 10, 'world']

Usage:

* ``%store``          - Show list of all variables and their current
                        values
* ``%store spam bar`` - Store the *current* value of the variables spam
                        and bar to disk
* ``%store -d spam``  - Remove the variable and its value from storage
* ``%store -z``       - Remove all variables from storage
* ``%store -r``       - Refresh all variables, aliases and directory history
                        from store (overwrite current vals)
* ``%store -r spam bar`` - Refresh specified variables and aliases from store
                           (delete current val)
* ``%store foo >a.txt``  - Store value of foo to new file a.txt
* ``%store foo >>a.txt`` - Append value of foo to file a.txt

It should be noted that if you change the value of a variable, you
need to %store it again if you want to persist the new value.

Note also that the variables will need to be pickleable; most basic
python types can be safely %store'd.

Also aliases can be %store'd across sessions.
To remove an alias from the storage, use the %unalias magic.
File:      ~/.virtualenvs/tmp/lib/python3.11/site-packages/IPython/extensions/storemagic.py

This way it is guaranteed that the same types are being used, which were
actually different. The `--set-default` flag now also gets its default
from the current value on the selected user, just as is done for the
other properties.
The user options `--first-name`, `--last-name` and `--institution` in
the `verdi quicksetup/setup` commands were recently made required but
did not provide a default. This would make creating profiles
significantly more complex than always needed. For simple test and demo
profiles the user might not necessarily care about these user details.

Here we add defaults for these options. Even for production profiles
this is a sensible approach since these details can always be freely
updated later on with `verdi user configure`. This is also the reason
that the `--email` does not provide a default because that can not be
changed later on.
Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sphuber ok, I reviewed the commits one by one and have no further comments. So this one is good to go for me! 🚀

@sphuber sphuber force-pushed the feature/mutable-users branch from 99f8609 to 2bb6128 Compare November 12, 2023 19:02
@sphuber sphuber merged commit 8b8887e into aiidateam:main Nov 12, 2023
17 checks passed
@sphuber sphuber deleted the feature/mutable-users branch November 12, 2023 21:03
@sphuber
Copy link
Contributor Author

sphuber commented Nov 12, 2023

Thanks a lot for the review @mbercx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants